Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve blame file type detection (#1803) #1829

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dvermd
Copy link
Contributor

@dvermd dvermd commented Aug 26, 2024

closes #1803

Comment on lines 728 to 731

let args = make_string_vec(&["git", "blame", "-w", "-C", "main.rs"]);
assert_eq!(guess_git_blame_filename(&args), Args("main.rs".into()));

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That unfortunately doesn't really fix it: Now -w takes an argument (when it does not), which means it "eats" the following -C. The problem is -C[<num>], which only takes a non-whitespace argument (or none) -- but this not really modeled at the moment.

Removing -C from the previous list should work for most cases. In the future a fallback to take the last argument containing a . should be added. And make it use the CallingProcess enum as mentioned in the TODO there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been tricked by only looking at the synopsis part of the git blame man page when adding -w to this list.

I'll look at addressing this TODO and making a variant in CallingProcess.

@dvermd dvermd force-pushed the improve_blame_filetype_detection branch from 5392bc4 to c5bc7ed Compare September 10, 2024 04:20
@dvermd dvermd force-pushed the improve_blame_filetype_detection branch from c5bc7ed to aa78604 Compare September 15, 2024 13:47
Copy link
Collaborator

@th1000s th1000s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, a few small things.

src/utils/process.rs Outdated Show resolved Hide resolved
src/utils/process.rs Outdated Show resolved Hide resolved
src/handlers/blame.rs Outdated Show resolved Hide resolved
src/utils/process.rs Outdated Show resolved Hide resolved
src/utils/process.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 git blame -C loses syntax highlighting
2 participants